Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[R-package] [ci] switch vignettes from 'rmarkdown' to 'markdown' #6258

Merged
merged 3 commits into from
Jan 9, 2024

Conversation

jameslamb
Copy link
Collaborator

@jameslamb jameslamb commented Jan 6, 2024

Related to #1944.

Proposes switching from {rmarkdown} to {markdown} for the R package's vignettes.

Benefits of this Change

Reduces the set of dependencies that have to build successfully to run R CMD check --as-cran against {lightgbm}, leading to:

  • faster CI builds
  • lower risk of CI failures
  • lower risk of failed builds and disruptions on CRAN
  • slightly reduced reverse-dependency burden for CRAN and maintainers of {rmarkdown}
this change results in 14 fewer packages needing to be installed in CI

R code I used (with R 4.2.3) to calculate that:

CRAN_DB <- tools::CRAN_package_db()

.get_strong_deps <- function(packages) {
    ret <- tools::package_dependencies(
        packages = packages
        , db = CRAN_DB
        , which = c("Depends", "Imports", "LinkingTo")
        , recursive = TRUE
    )
    ret_vec <- c(unlist(ret), packages)
    return(sort(unique(ret_vec)))
}

# combination of the following:
#   * LightGBM's strong dependencies (recursive)
#   * LightGBM's "Suggests" dependencies
#   * strong dependencies of those "Suggests" dependencies (recursive)
#
# excluding {rmarkdown} / {markdown}
lgb_r_cmd_check_deps <- setdiff(
    .get_strong_deps(c("lightgbm", "knitr", "RhpcBLASctl", "testthat"))
    , "lightgbm"
)
length(lgb_r_cmd_check_deps)
# 47

strong_deps_for_rmarkdown <- .get_strong_deps("rmarkdown")
strong_deps_for_markdown <- .get_strong_deps("markdown")

# additional packages added by {rmarkdown}
setdiff(strong_deps_for_rmarkdown, lgb_r_cmd_check_deps)

# [1] "base64enc"   "bslib"       "cachem"      "ellipsis"   
# [5] "fastmap"     "fontawesome" "htmltools"   "jquerylib"  
# [9] "memoise"     "mime"        "rappdirs"    "rmarkdown"  
# [13] "sass"        "stringi"     "stringr"     "tinytex"

# additional packages added by {markdown}
setdiff(strong_deps_for_markdown, lgb_r_cmd_check_deps)
# [1] "commonmark" "markdown"

But what are we giving up?

My understanding is the main benefits of {rmarkdown} over {markdown} are things that are not relevant to the simple documentation {lightgbm} produces. Things like:

{markdown}, as stated in its README (https://github.com/rstudio/markdown), aims to be simpler and lighter-weight:

It is a deliberate design choice to keep this markdown package lightweight...
... this package is intended for minimalists

How I tested this

Enabled this branch on readthedocs and built the docs there.

The build took 807 seconds (build link), roughly the same as other builds took over the last 7 days (build history).

Confirmed that the rendered vignette looks correct at the documentation site: https://lightgbm.readthedocs.io/en/docs-lighter-weight-vignettes/R/articles/basic_walkthrough.html.

Many of this project's R CI jobs build the vignettes and test them with R CMD check --as-cran, so I'm confident this change wouldn't cause issues for CRAN.

Confirmed all references to {rmarkdown} have been replaced by {markdown}:

git grep -i rmarkdown

Notes for Reviewers

This PR inspired by @jangorecki's post on the r-pkg-devel mailing list: https://stat.ethz.ch/pipermail/r-package-devel/2024q1/010280.html. {data.table} recently made a similar change in pursuit of reducing the weight of their dependencies.

Related:

@jameslamb jameslamb changed the title WIP: [R-package] [ci] switch vignettes from 'rmarkdown' to 'markdown' [R-package] [ci] switch vignettes from 'rmarkdown' to 'markdown' Jan 6, 2024
@jameslamb jameslamb marked this pull request as ready for review January 6, 2024 23:33
@@ -9,9 +9,9 @@ dependencies:
- r-data.table=1.14.2
- r-jsonlite=1.7.2
- r-knitr=1.37
- r-markdown
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm planning a follow-up PR updating the version constraints on all the conda packages we pull in in this project's CI. I'll put an constraint on r-markdown then.

For now, I think it's fine to let this float and have the conda solver figure out an appropriate version.

@jangorecki
Copy link

jangorecki commented Jan 7, 2024

Such change also removes the need for OS dep pandoc (180mb) because markdown package does not use pandoc.

@jameslamb
Copy link
Collaborator Author

Thanks! Just pushed f3ac92a removing pandoc too 😁

@jameslamb
Copy link
Collaborator Author

Seems that R CMD CHECK --as-cran still requires pandoc.

* checking top-level files ... NOTE
Files ‘README.md’ or ‘NEWS.md’ cannot be checked without ‘pandoc’ being installed.

@jangorecki
Copy link

Yes, it is a build environment that doesn't need pandoc anymore, test environment will still need it for the reason you wrote.

This reverts commit f3ac92a.
@jameslamb jameslamb merged commit 03ee995 into master Jan 9, 2024
41 checks passed
@jameslamb jameslamb deleted the docs/lighter-weight-vignettes branch January 9, 2024 03:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants